feat(appsync): allow user to configure log retention time #21418

nikovirtala · 2022-08-02T08:07:08Z

This pull request implements a feature that allows users to configure the log retention period for App Sync logs.

AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention.

The log group is named following the /aws/appsync/apis/{graphql_api_id} format

ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html

At the same time, AWS CDK provides a stable construct to set log retention times for any log group.

Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do.

This is the continuation of work done in #20536 – this time with documentation and integration test 😄

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-08-02T08:07:16Z

nikovirtala · 2022-08-02T08:12:53Z

I have a problem executing the integration tests – it doesn't recognize my AWS account credential configuration and thus fails to deploy the test stack.

I tried to look around but couldn't find any hints on how the environment should be configured to make these tests work. So, what should I do?

As you can see, the CDK thinks my account and region are unknown:

yarn integ-runner --update-on-failed
yarn run v1.22.15
$ /Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/node_modules/.bin/integ-runner --update-on-failed

Verifying integration test snapshots...

  NEW        integ.log-retention 0.559s
  UNCHANGED  integ.api-import 1.265s
  UNCHANGED  integ.appsync-lambda 1.324s
  UNCHANGED  integ.auth-apikey 2.514s
  UNCHANGED  integ.graphql-iam 2.495s
  UNCHANGED  integ.graphql-schema 2.51s
  UNCHANGED  integ.graphql-elasticsearch 2.536s
  UNCHANGED  integ.graphql-opensearch 2.675s
  UNCHANGED  integ.graphql 2.86s

Snapshot Results:

Tests:    1 failed, 9 total

Running integration tests for failed tests...

Running in parallel across regions: us-east-1, us-east-2, us-west-2
Running test /Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/@aws-cdk/aws-appsync/test/integ.log-retention.js in us-east-1
Selecting stack by identifier "IntegDefaultTestDeployAssert4E6713E1". This identifier is deprecated and will be removed in v2. Please use "Integ/DefaultTest/DeployAssert" instead.
Run "cdk ls" to see a list of all stack identifiers

✨  Synthesis time: 0.19s

AppSyncIntegLogRetention
AppSyncIntegLogRetention: deploying...

 ❌  AppSyncIntegLogRetention failed: Error: This stack uses assets, so the toolkit stack must be deployed to the environment (Run "cdk bootstrap aws://unknown-account/unknown-region")
    at Object.addMetadataAssetsToManifest (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/assets.ts:27:11)
    at Object.deployStack (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/api/deploy-stack.ts:237:29)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at CdkToolkit.deploy (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/cdk-toolkit.ts:219:24)
    at initCommandLine (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/aws-cdk/lib/cli.ts:347:12)

This stack uses assets, so the toolkit stack must be deployed to the environment (Run "cdk bootstrap aws://unknown-account/unknown-region")
  FAILED     integ.log-retention-Integ/DefaultTest (undefined/us-east-1) 4.26s
      Integration test failed: Error: Command exited with status 1

Test Results:

Tests:    1 failed, 1 total
Error: Some integration tests failed!
    at main (/Users/nikovirtala/src/github.com/nikovirtala/aws-cdk/packages/@aws-cdk/integ-runner/lib/cli.js:105:23)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

mrgrain · 2022-08-02T09:23:02Z

Hi @nikovirtala and thanks for looking into the integration tests! It depends a bit on how your credentials are configured locally, but I reckon the easiest way to influence the behaviour of running the test, will be env vars.

E.g. you can run it like this:

AWS_PROFILE=your_profile AWS_REGION=us-east-1 yarn integ

There are a few more variables you can set depending on your exact setup. Please let me know if that helps.

otaviomacedo · 2022-08-02T10:29:15Z

@nikovirtala When I run the new integration test, the deployment fails with "The fieldLogLevel is invalid".

I believe this is a bug in the AppSync control plane. The documentation says that FieldLogLevel is optional, but if you provide a LogConfig in the CloudFormation template without passing a FieldLogLevel, you get this error. And this is what happens with your change:

"LogConfig": {
  "CloudWatchLogsRoleArn": {
    "Fn::GetAtt": [
      "GraphqlApiApiLogsRoleBB9E6BAD",
      "Arn"
    ]
  }
}

Despite being a bug in the backend, we can default the value of this field to NONE, to avoid a failure.

packages/@aws-cdk/aws-appsync/README.md

Pull request has been modified.

nikovirtala · 2022-08-02T10:51:38Z

Despite being a bug in the backend, we can default the value of this field to NONE, to avoid a failure.

I noticed that too but was focused to get the integration tests to run against my environment. It is fixed now.

TheRealAmazonKendra · 2022-08-02T15:31:34Z

If you have too much trouble getting the tests to run, we can run it for you and have you double check that the output is what you'd expect. Granted, it's much better to get your setup working so that you can run tests in the future as well, but feel free to @ me if you need some additional guidance/assistance on that.

TheRealAmazonKendra · 2022-08-02T15:34:12Z

@nikovirtala If you haven't already done so, follow these instructions for your local setup Configuring the AWS CLI.

TheRealAmazonKendra

Just a few small comments from me inline.

packages/@aws-cdk/aws-appsync/README.md

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

packages/@aws-cdk/aws-appsync/test/integ.log-retention.ts

nikovirtala · 2022-08-03T06:35:37Z

If you have too much trouble getting the tests to run, we can run it for you and have you double check that the output is what you'd expect. Granted, it's much better to get your setup working so that you can run tests in the future as well, but feel free to @ me if you need some additional guidance/assistance on that.

@TheRealAmazonKendra It's not a huge problem for me to spend some time to figure out why I cannot make the tests run – I seriously doubt that this won't be the last time I need to touch 'em here or on some other package 😉

@mrgrain gave me some ideas on what to try next at cdk.dev Slack yesterday ... going to try those next.

Pull request has been modified.

packages/@aws-cdk/aws-appsync/README.md

packages/@aws-cdk/aws-appsync/test/integ.log-retention.ts

Pull request has been modified.

packages/@aws-cdk/aws-appsync/README.md

TheRealAmazonKendra · 2022-08-03T17:37:56Z

FWIW, I ran the integ tests on my machine, out of curiosity, and they all passed.

Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com>

Pull request has been modified.

TheRealAmazonKendra · 2022-08-04T06:59:30Z

@Mergifyio update

mergify · 2022-08-04T07:00:03Z

update

☑️ Nothing to do

-closed [:pushpin: update requirement]
#commits-behind>0 [:pushpin: update requirement]

nikovirtala · 2022-08-04T07:33:22Z

FWIW, I ran the integ tests on my machine, out of curiosity, and they all passed.

I know the test I added passes in its current state but IMO such a test without an assertion doesn't make much sense 😄

Now that I know how to successfully deploy the changed tests, I can add the assertion.

nikovirtala · 2022-08-04T11:40:20Z

The integration test result is now verified using AwsApiCall - which is pretty neat feature, to be honest 👌🏼

mergify · 2022-08-04T14:12:10Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-04T14:49:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: ac64ec6
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-04T14:49:55Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This pull request implements a feature that allows users to configure the log retention period for App Sync logs. AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention. > The log group is named following the /aws/appsync/apis/{graphql_api_id} format ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html At the same time, AWS CDK provides a stable [construct](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_logs.LogRetention.html) to set log retention times for any log group. Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do. This is the continuation of work done in aws#20536 – this time with documentation and integration test 😄 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Aug 2, 2022

aws-cdk-automation requested a review from a team August 2, 2022 08:08

nikovirtala force-pushed the appsync-log-retention branch 3 times, most recently from 03273a0 to 0fa8467 Compare August 2, 2022 08:28

mrgrain changed the title ~~feat(appsync): allow user to configure log retention time~~ feat(aws-appsync): allow user to configure log retention time Aug 2, 2022

mrgrain changed the title ~~feat(aws-appsync): allow user to configure log retention time~~ feat(appsync): allow user to configure log retention time Aug 2, 2022

otaviomacedo previously requested changes Aug 2, 2022

View reviewed changes

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved

nikovirtala force-pushed the appsync-log-retention branch from d102d3f to 11291b0 Compare August 3, 2022 06:13

TheRealAmazonKendra previously requested changes Aug 3, 2022

View reviewed changes

nikovirtala force-pushed the appsync-log-retention branch from 11291b0 to 7a96bdc Compare August 3, 2022 07:12

TheRealAmazonKendra previously requested changes Aug 3, 2022

View reviewed changes

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-appsync/test/integ.log-retention.ts Show resolved Hide resolved

TheRealAmazonKendra previously requested changes Aug 3, 2022

View reviewed changes

packages/@aws-cdk/aws-appsync/README.md Outdated Show resolved Hide resolved

nikovirtala and others added 5 commits August 4, 2022 09:36

feat(appsync): allow user to configure log retention time

79bfff0

Update packages/@aws-cdk/aws-appsync/README.md

1472b31

Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com>

fix: set field log level to none

60741dd

fix: retention period default

dd7acfd

docs: add an example of log retention

4846503

nikovirtala added 2 commits August 4, 2022 09:36

docs: improved example of log retention

8bb4d6a

fix(docs): fix module imports in code example

b8979b4

nikovirtala force-pushed the appsync-log-retention branch from 61811bf to b8979b4 Compare August 4, 2022 06:37

nikovirtala force-pushed the appsync-log-retention branch from 01026db to e198871 Compare August 4, 2022 08:44

fix(docs): it is appsync log config ...

46fc710

nikovirtala force-pushed the appsync-log-retention branch from e198871 to 46fc710 Compare August 4, 2022 09:52

update integration test with assertion

fe6bf6f

test: no log retention defined

ea5303d

TheRealAmazonKendra approved these changes Aug 4, 2022

View reviewed changes

Merge branch 'main' into appsync-log-retention

ac64ec6

mergify bot merged commit a2bb263 into aws:main Aug 4, 2022

nikovirtala deleted the appsync-log-retention branch August 5, 2022 10:04

mrgrain added the effort/medium Medium work item – several days of effort label Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appsync): allow user to configure log retention time #21418

feat(appsync): allow user to configure log retention time #21418

nikovirtala commented Aug 2, 2022

gitpod-io bot commented Aug 2, 2022

nikovirtala commented Aug 2, 2022

mrgrain commented Aug 2, 2022 •

edited

Loading

otaviomacedo commented Aug 2, 2022

nikovirtala commented Aug 2, 2022

TheRealAmazonKendra commented Aug 2, 2022

TheRealAmazonKendra commented Aug 2, 2022

TheRealAmazonKendra left a comment

nikovirtala commented Aug 3, 2022

TheRealAmazonKendra commented Aug 3, 2022

TheRealAmazonKendra commented Aug 4, 2022

mergify bot commented Aug 4, 2022

nikovirtala commented Aug 4, 2022

nikovirtala commented Aug 4, 2022 •

edited

Loading

mergify bot commented Aug 4, 2022

aws-cdk-automation commented Aug 4, 2022

mergify bot commented Aug 4, 2022

feat(appsync): allow user to configure log retention time #21418

feat(appsync): allow user to configure log retention time #21418

Conversation

nikovirtala commented Aug 2, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 2, 2022

nikovirtala commented Aug 2, 2022

mrgrain commented Aug 2, 2022 • edited Loading

otaviomacedo commented Aug 2, 2022

nikovirtala commented Aug 2, 2022

TheRealAmazonKendra commented Aug 2, 2022

TheRealAmazonKendra commented Aug 2, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

nikovirtala commented Aug 3, 2022

TheRealAmazonKendra commented Aug 3, 2022

TheRealAmazonKendra commented Aug 4, 2022

mergify bot commented Aug 4, 2022

☑️ Nothing to do

nikovirtala commented Aug 4, 2022

nikovirtala commented Aug 4, 2022 • edited Loading

mergify bot commented Aug 4, 2022

aws-cdk-automation commented Aug 4, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 4, 2022

mrgrain commented Aug 2, 2022 •

edited

Loading

nikovirtala commented Aug 4, 2022 •

edited

Loading